PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

internal async Task UninstallAsync(Extension extension, UninstallOptions? options = null)
{
    var @params = new UninstallCommandParameters(extension);

    await Broker.ExecuteCommandAsync(new UninstallCommand(@params), options).ConfigureAwait(false);
}

PR Code Suggestions ✨

Explore these optional code suggestions:

-public record InstallResult(Extension Extension) : EmptyResult;
+public record InstallResult(Extension Extension);

Test fails on CI, because I don't know how to prepare test data in bazel test environment. It passes locally in IDE and dotnet test.

[IgnoreBrowser(Selenium.Browser.Chrome, "Web extensions are not supported yet?")]
[IgnoreBrowser(Selenium.Browser.Edge, "Web extensions are not supported yet?")]

They should work for Chrome/Edge if you start the browser using the flags:

--remote-debugging-pipe
--enable-unsafe-extension-debugging

The Python bindings added a property on Chromium's Options class named enable_webextensions that a user can enable to set these flags.

see #15794

Test fails on CI, because I don't know how to prepare test data in bazel test environment. It passes locally in IDE and dotnet test.

I managed it and now we are able to execute tests in different environments (dotnet test, bazel test, IDE).

--remote-debugging-pipe argument breaks CDP.

#15749 (comment)

Parking it until better weather.

Here we should refactor tests projects. Any test Fixture should be capable to override the initialization of WebDriver. That's it. Probably relates to #15536

Hi Team,

Will this option be available in the upcoming Selenium version 4.35?

Thanks.

@SKumar-777 no, sorry... 4.35 was released yesterday and this hasn't been merged yet.

Sorry, I am stuck here. Options to move further:

• Refactor internal tests infrastructure (big effort)
• Apply some dirty hack to support this specific case
• Temporary ignore internal tests and release the functionality

@navin772 @Delta456 what approach did you take when implementing this?

Refactor internal tests infrastructure (big effort)

what approach did you take when implementing this?

Python tests and fixtures were updated to handle this.

@navin772 @Delta456 what approach did you take when implementing this?

The extension related files/dirs had to be copied via bazel, which this PR is already doing.
@nvborisenko I don't have much idea about dotnet but what exactly is blocking this PR? The implementation looks good. Are we blocked on tests, in the last CI run, I see that the related tests are passing.

Ideally to move further we should be able to add arguments for specific fixture/suite/test.

--remote-debugging-pipe
--enable-unsafe-extension-debugging

The issue is that in internal implemented testing infrastructure we cannot do it, only globally, but it breaks DevTools/CDP. Looked at the code, and I don't even see "quick dirty hack" to improve the situation.

Seems our option is to merge without tests, and then add them later when weather becomes good: #15536

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

public override Extension? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
    var id = reader.GetString();

    return new Extension(_bidi, id!);
}

public override void Write(Utf8JsonWriter writer, Extension value, JsonSerializerOptions options)
{
    writer.WriteStringValue(value.Id);
}

[JsonPolymorphic(TypeDiscriminatorPropertyName = "type")]
[JsonDerivedType(typeof(ExtensionArchivePath), "archivePath")]
[JsonDerivedType(typeof(ExtensionBase64Encoded), "base64")]
[JsonDerivedType(typeof(ExtensionPath), "path")]
public abstract record ExtensionData;

public sealed record ExtensionArchivePath(string Path) : ExtensionData;

public sealed record ExtensionBase64Encoded(string Value) : ExtensionData;

public sealed record ExtensionPath(string Path) : ExtensionData;

[Ignore("""
    The following test suite wants to set driver arguments via Options, but it breaks CDP/DevTools tests.
    The desired arguments (for Chromium only?):
    --enable-unsafe-extension-debugging
    --remote-debugging-pipe
    Ignoring these tests for now. Hopefully https://github.com/SeleniumHQ/selenium/issues/15536 will be resolved soon.
    """)]
class WebExtensionTest : BiDiTestFixture
{
    [Test]

PR Code Suggestions ✨

Explore these optional code suggestions:

public WebExtension.WebExtensionModule WebExtension
{
    get
    {
        if (_webExtensionModule is not null) return _webExtensionModule;
        lock (_moduleLock)
        {
            _webExtensionModule ??= new WebExtension.WebExtensionModule(_broker);
        }
        return _webExtensionModule;

 ... (clipped 2 lines)

public async Task<InstallResult> InstallAsync(ExtensionData extensionData, InstallOptions? options = null)
{
    var @params = new InstallCommandParameters(extensionData);

    return await Broker.ExecuteCommandAsync<InstallCommand, InstallResult>(new InstallCommand(@params), options).ConfigureAwait(false);
}

// In BiDi.cs
public WebExtension.WebExtensionModule WebExtension
{
    get
    {
        // Lazily initializes module without checking for browser support.
        if (_webExtensionModule is not null) return _webExtensionModule;
        lock (_moduleLock)
        {
            _webExtensionModule ??= new WebExtension.WebExtensionModule(_broker);
        }
        return _webExtensionModule;
    }
}

// In BiDi.cs
// Assumes _supportedModules is populated from session.status on connection.
private HashSet<string> _supportedModules;

public WebExtension.WebExtensionModule WebExtension
{
    get
    {
        if (!_supportedModules.Contains("webExtension"))
        {
            throw new WebDriverException("The WebExtension BiDi module is not supported by this browser.");
        }
        // Lazily initialize module only if supported.
        _webExtensionModule ??= new WebExtension.WebExtensionModule(_broker);
        return _webExtensionModule;
    }
}

 public override Extension? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
 {
+    if (reader.TokenType != JsonTokenType.String)
+    {
+        throw new JsonException($"Expected string for Extension id, but got {reader.TokenType}.");
+    }
+
     var id = reader.GetString();
+    if (string.IsNullOrEmpty(id))
+    {
+        throw new JsonException("Extension id cannot be null or empty.");
+    }
 
-    return new Extension(_bidi, id!);
+    return new Extension(_bidi, id);
 }

 [JsonPolymorphic(TypeDiscriminatorPropertyName = "type")]
 [JsonDerivedType(typeof(ExtensionArchivePath), "archivePath")]
 [JsonDerivedType(typeof(ExtensionBase64Encoded), "base64")]
 [JsonDerivedType(typeof(ExtensionPath), "path")]
 public abstract record ExtensionData;
 
-public sealed record ExtensionArchivePath(string Path) : ExtensionData;
+public sealed record ExtensionArchivePath([property: JsonPropertyName("path")] string Path) : ExtensionData;
 
-public sealed record ExtensionBase64Encoded(string Value) : ExtensionData;
+public sealed record ExtensionBase64Encoded([property: JsonPropertyName("value")] string Value) : ExtensionData;
 
-public sealed record ExtensionPath(string Path) : ExtensionData;
+public sealed record ExtensionPath([property: JsonPropertyName("path")] string Path) : ExtensionData;

 public async Task<InstallResult> InstallAsync(ExtensionData extensionData, InstallOptions? options = null)
 {
+    if (extensionData is null) throw new ArgumentNullException(nameof(extensionData));
+
     var @params = new InstallCommandParameters(extensionData);
-
     return await Broker.ExecuteCommandAsync<InstallCommand, InstallResult>(new InstallCommand(@params), options).ConfigureAwait(false);
 }

 public Extension(BiDi bidi, string id)
 {
-    _bidi = bidi;
-    Id = id;
+    _bidi = bidi ?? throw new ArgumentNullException(nameof(bidi));
+    Id = !string.IsNullOrEmpty(id) ? id : throw new ArgumentException("Extension id cannot be null or empty.", nameof(id));
 }

Let's merge it with the proved assumptions:

• it works in general
• test data works, bazel + IDE

Some tests are written, but just ignore for now. Show must go on.

CI failure is not related to these changes. I am merging it because users wait this functionality, even untested.